Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle VS Build Tools 2022 when trying to detect VS version. #186

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gabware
Copy link

@gabware gabware commented May 10, 2023

This only handles cases where the user explicitely set BAZEL_VC through the environment.
Handling BAZEL_VS will require more changes I think.
LMKWYT.

@gabware
Copy link
Author

gabware commented May 11, 2023

Added code to handle default locations for VS installation.
Tried to minimize code duplication by generating the list of locations.

@oquenchil oquenchil self-assigned this May 15, 2023
@oquenchil
Copy link
Contributor

Could you please send a PR for this to the copy of the file in bazelbuild/bazel's bazel_tools so that they are less out of sync? Thanks

@gabware
Copy link
Author

gabware commented May 26, 2023

Sure. Couple questions before I do that:

  • We're talking about updating https://github.com/bazelbuild/bazel/blob/master/tools/cpp/windows_cc_configure.bzl . Correct?
  • How do I test that I didn't break anything ? What are the targets I should be building ?
  • I see there are quite a few differences between the two files. Is it ok if I redo only the changes from this PR? I think it'd be best to avoid potential build break and keep PR small and consistent.

@oquenchil
Copy link
Contributor

We're talking about updating https://github.com/bazelbuild/bazel/blob/master/tools/cpp/windows_cc_configure.bzl . Correct?

Yes.

How do I test that I didn't break anything ? What are the targets I should be building ?

We don't test the toolchain files themselves. But if nothing breaks on CI then we're good.

I see there are quite a few differences between the two files. Is it ok if I redo only the changes from this PR? I think it'd be best to avoid potential build break and keep PR small and consistent.

Absolutely, only the changes from this PR. But I'm even starting to hesitate whether you should spend time on that.

@fmeum What did we decide a few months back that should eventually be source of truth? Was it the version in bazel_tools or the one in rules_cc? Do you think it makes sense Gabriel to update both here?

@fmeum
Copy link
Contributor

fmeum commented May 30, 2023

CC @meteorcloudy, who is working on making rules_java the source of truth for Java toolchains (see bazelbuild/bazel#18423).

Given that, I'm pretty sure the correct answer is rules_cc.

@meteorcloudy
Copy link
Member

bazelbuild/bazel#18423 is almost done, and looks like a success. I think we should probably do the same for rules_cc, rules_android to remove more stuff from @bazel_tools.

@comius @ted-xie

@meteorcloudy
Copy link
Member

meteorcloudy commented May 30, 2023

I can take a look at using rules_cc as source of truth after finishing rules_java

@gabware
Copy link
Author

gabware commented May 30, 2023

Was it the version in bazel_tools or the one in rules_cc?

AFAICT the version in rules_cc seems cleaner; better maintained. Functions were moved to private, there are a couple fixes in, etc. I believe rules_cc should be the source of truth.

Do you think it makes sense Gabriel to update both here?
Unless I'm mistaken, I'd have to do a different PR because https://github.com/bazelbuild/bazel is a different repo.
So I won't "update both here" per-se. I'll update both as two PR. (unless there's a way to update both here that I don't know; I'm not super familiar with github yet).

@meteorcloudy
Copy link
Member

@gabware Yes, I think Pedro meant we should also update the toolchain configuration in the Bazel repo to keep this in sync until we actually switch to rules_cc as the source of truth (meaning loading toolchains from rules_cc by default in Bazel)

@oquenchil
Copy link
Contributor

Sounds like we can get this in then.

@gabware
Copy link
Author

gabware commented Jun 19, 2023

Anything left here or can we merge this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants